Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Update location_background #906

Merged
merged 3 commits into from
Nov 19, 2018
Merged

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Nov 19, 2018

FlutterHeadlessDartRunner is now deprecated and effectively a typedef for FlutterEngine.

One method got renamed. This PR also adds some guards around API calls that need it.

This will have to be landed on red to fix the build.

@dnfield dnfield requested review from amirh and bkonyi November 19, 2018 20:22
@@ -194,12 +197,14 @@ - (void)monitorLocationChanges:(NSArray *)arguments {
_locationManager.pausesLocationUpdatesAutomatically = arguments[1];
if (@available(iOS 11.0, *)) {
_locationManager.showsBackgroundLocationIndicator = arguments[2];
[self setShowsBackgroundLocationIndicator:_locationManager.showsBackgroundLocationIndicator];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkonyi - is it safe to move this up here? It seemed like it, but wanted to make sure there weren't expectations of setting any other state before calling this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this should be fine. It's just saving the state to reset the location manager object, so ordering shouldn't really matter.

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit and a question 😄

@@ -8,7 +8,7 @@

@implementation LocationBackgroundPlugin {
CLLocationManager *_locationManager;
FlutterHeadlessDartRunner *_headlessRunner;
FlutterEngine *_headlessRunner;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we rename the variable as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, I didn't have strong opinions on it. It is still meant to be headless, never getting a Viewcontroller attached to it at present AFAIK

@@ -48,7 +48,9 @@ - (BOOL)application:(UIApplication *)application
_locationManager.showsBackgroundLocationIndicator =
[self getShowsBackgroundLocationIndicator];
}
_locationManager.allowsBackgroundLocationUpdates = YES;
if (@available(iOS 9.0, *)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkonyi What should we do when this is not supported? throw an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error or warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this isn't supported that parameter is ignored (pretty sure it's mentioned in the Dart docs).

@dnfield dnfield merged commit 78e23e4 into flutter:master Nov 19, 2018
andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
* Update location_background
andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants